Skip to content

Fix #438, add three PDI deactivation option through CMake #543

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 387 commits into
base: main
Choose a base branch
from
Open

Fix #438, add three PDI deactivation option through CMake #543

wants to merge 387 commits into from

Conversation

@JAuriac
Copy link
Contributor

@JAuriac JAuriac commented Feb 20, 2025 •
edited
Loading

Missing the case where paraconf is missing.

List of things to check before making a PR

Before merging your code, please check the following:

  • you have added a line describing your changes to the Changelog;
  • you have added unit tests for any new or improved feature;
  • In case you updated dependencies, you have checked pdi/docs/CheckList.md
  • you have checked your code format:
    • you have checked that you respect all conventions specified in CONTRIBUTING.md;
    • you have checked that the indentation and formatting conforms to the .clang-format;
    • you have documented with doxygen any new or changed function / class;
  • you have correctly updated the copyright headers:
    • your institution is in the copyright header of every file you (substantially) modified;
    • you have checked that the end-year of the copyright there is the current one;
  • you have updated the AUTHORS file:
    • you have added yourself to the AUTHORS file;
    • if this is a new contribution, you have added it to the AUTHORS file;
  • you have added everything to the user documentation:
    • any new CMake configuration option;
    • any change in the yaml config;
    • any change to the public or plugin API;
    • any other new or changed user-facing feature;
    • any change to the dependencies;
  • you have correctly linked your MR to one or more issues:
    • your MR solves an identified issue;
    • your commit contain the Fix #issue keyword to autoclose the issue when merged.

@JAuriac JAuriac linked an issue Feb 20, 2025 that may be closed by this pull request
Provide a mechanism to deactivate PDI on demand #438
Open
@jbigot
Copy link
Member

jbigot commented Feb 24, 2025

Warning you're updating the tutorial. This is unrelated

benoitmartin88
benoitmartin88 requested changes Feb 24, 2025
View reviewed changes
Copy link
Member

@benoitmartin88 benoitmartin88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this PR is almost ready for merge. Just a few changes are needed.

@JAuriac
Copy link
Contributor Author

JAuriac commented Feb 25, 2025

Warning you're updating the tutorial. This is unrelated

The tutorial previously used broke the CI.

@JAuriac
Copy link
Contributor Author

JAuriac commented Feb 26, 2025

Warning you're updating the tutorial. This is unrelated

The tutorial previously used broke the CI.

Changing back to tutorial @ 7334107.

jbigot
jbigot reviewed Feb 26, 2025
View reviewed changes
jbigot
jbigot reviewed Feb 26, 2025
View reviewed changes
jbigot
jbigot reviewed Feb 26, 2025
View reviewed changes
jbigot
jbigot reviewed Feb 26, 2025
View reviewed changes
jbigot
jbigot reviewed Feb 26, 2025
View reviewed changes
jbigot
jbigot reviewed Feb 26, 2025
View reviewed changes
jbigot
jbigot reviewed Feb 26, 2025
View reviewed changes
jbigot
jbigot reviewed Feb 26, 2025
View reviewed changes
jbigot
jbigot reviewed Feb 26, 2025
View reviewed changes
jbigot
jbigot reviewed Feb 26, 2025
View reviewed changes
jbigot
jbigot reviewed Feb 26, 2025
View reviewed changes
jbigot
jbigot reviewed Feb 26, 2025
View reviewed changes
jbigot
jbigot reviewed Feb 26, 2025
View reviewed changes
jbigot
jbigot reviewed Feb 26, 2025
View reviewed changes
jbigot
jbigot reviewed Feb 26, 2025
View reviewed changes
jbigot
jbigot reviewed Feb 26, 2025
View reviewed changes
jbigot
jbigot reviewed Feb 26, 2025
View reviewed changes
jbigot
jbigot reviewed Feb 26, 2025
View reviewed changes
jbigot
jbigot reviewed Feb 26, 2025
View reviewed changes
jbigot
jbigot reviewed Feb 26, 2025
View reviewed changes
jbigot
jbigot reviewed Feb 26, 2025
View reviewed changes
JAuriac added 25 commits October 30, 2025 14:10
@JAuriac
Attempt to enable multiple ctest junit output files for the CI, via m…
a2d0dae 
…anual merge using sed 6
@JAuriac
Attempt to enable multiple ctest junit output files for the CI, via m…
c303700 
…anual merge using sed 7
@JAuriac
Attempt to enable multiple ctest junit output files for the CI, via m…
513501e 
…anual merge using sed 8
@JAuriac
Attempt to enable multiple ctest junit output files for the CI, via m…
922f158 
…anual merge using sed 9
@JAuriac
Attempt to enable multiple ctest junit output files for the CI, via m…
ea775f9 
…anual merge using sed 10
@JAuriac
Attempt to enable multiple ctest junit output files for the CI, via m…
da0a6df 
…anual merge using sed and tar
@JAuriac
Attempt to enable multiple ctest junit output files for the CI, via m…
77b5d50 
…anual merge using sed and tar 2
@JAuriac
Attempt to enable multiple ctest junit output files for the CI, via m…
62bbf52 
…anual merge using sed and tar 3
@JAuriac
Attempt to enable multiple ctest junit output files for the CI, via m…
9b9e1ac 
…anual merge using sed and tar 4
@JAuriac
Attempt to enable multiple ctest junit output files for the CI, via m…
5512329 
…anual merge using sed and tar 5
@JAuriac
Attempt to enable multiple ctest junit output files for the CI, via m…
03a6db5 
…anual merge using sed and tar 6
@JAuriac
Attempt to enable multiple ctest junit output files for the CI, via m…
0c3b806 
…anual merge using sed and tar 7
@JAuriac
Attempt to enable multiple ctest junit output files for the CI, via m…
40338f3 
…anual merge using sed and tar 8
@JAuriac
Attempt to enable multiple ctest junit output files for the CI, via m…
2489a27 
…anual merge using sed and tar 9
@JAuriac
Attempt to enable multiple ctest junit output files for the CI, via m…
c40bb13 
…anual merge using sed and tar 10
@JAuriac
Attempt to enable multiple ctest junit output files for the CI, via m…
d264058 
…anual merge using sed and tar 11
@JAuriac
Attempt to enable multiple ctest junit output files for the CI, via m…
5fb8ca5 
…anual merge using sed and tar 12
@JAuriac
Attempt to enable multiple ctest junit output files for the CI, via m…
a0145e8 
…anual merge using sed and tar 13
@JAuriac
Attempt to enable multiple ctest junit output files for the CI, via m…
9cad52f 
…anual merge using sed and tar 14
@JAuriac
Attempt to enable multiple ctest junit output files for the CI, via m…
fb3239f 
…anual merge using sed and tar 15
@JAuriac
Attempt to enable multiple ctest junit output files for the CI, via m…
2e18f18 
…anual merge using sed and tar 16
@JAuriac
Attempt to enable multiple ctest junit output files for the CI, via m…
953da81 
…anual merge using sed and tar 17
@JAuriac
Attempt to enable multiple ctest junit output files for the CI, via j…
0b892a3 
…unitparser
@JAuriac
Revert to state of first ctest junit merge attempt, then attempt to e…
122b426 
…nable multiple ctest junit output files for the CI, without merge
@JAuriac
Ready for PR, cleaning
5a743f0
@JAuriac
Copy link
Contributor Author

JAuriac commented Nov 3, 2025

ubuntu/focal does not produce junit reports, most likely since the fix of the tmpfs/tests.xml.
https://github.com/pdidev/pdi/actions/runs/19039767522?pr=543#summary-54376281968
That aside, this PR's CI run successfully.

jbigot
jbigot reviewed Nov 4, 2025
View reviewed changes
Copy link
Member

@jbigot jbigot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, this is converging!

bin/build_and_run_all_tests
cmake -DDIST_PROFILE=Devel ${CMAKE_FLAGS} "${SRCDIR}"
make ${MAKEFLAGS}
ctest --output-on-failure --timeout 90 ${CTEST_FLAGS} ${EXCLUDED_PDI_TESTS:+-E $EXCLUDED_PDI_TESTS}
ctest --output-on-failure --timeout 90 --output-junit /tmp/tests_pdi.xml ${EXCLUDED_PDI_TESTS:+-E $EXCLUDED_PDI_TESTS}
Copy link
Member

@jbigot jbigot Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please do not remove ${CTEST_FLAGS} other users of the script might use it.

I am also not convinced about the fixed path for junit output.

Maybe:

Suggested change
ctest --output-on-failure --timeout 90 --output-junit /tmp/tests_pdi.xml ${EXCLUDED_PDI_TESTS:+-E $EXCLUDED_PDI_TESTS}
ctest --output-on-failure --timeout 90 ${CTEST_FLAGS} ${JUNIT_OUTPUT_DIR:+--output-junit ${JUNIT_OUTPUT_DIR}/tests_pdi.xml} ${EXCLUDED_PDI_TESTS:+-E $EXCLUDED_PDI_TESTS}

bin/build_and_run_all_tests
cd "${TEST_DIR_NO_PDI}"
cmake -DCMAKE_PREFIX_PATH="${SRCDIR}/no-pdi" "${SRCDIR}/example"
make ${MAKEFLAGS}
ctest --output-on-failure --timeout 90 --output-junit /tmp/tests_no-pdi_example.xml -R PDI_example_trace_C
Copy link
Member

@jbigot jbigot Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ctest --output-on-failure --timeout 90 --output-junit /tmp/tests_no-pdi_example.xml -R PDI_example_trace_C
ctest --output-on-failure --timeout 90 ${CTEST_FLAGS} ${JUNIT_OUTPUT_DIR:+--output-junit ${JUNIT_OUTPUT_DIR}/tests_no-pdi_example.xml} ${EXCLUDED_PDI_TESTS:+-E $EXCLUDED_PDI_TESTS}

bin/build_and_run_all_tests
cd "${TEST_DIR_NO_PDI}"
cmake -DCMAKE_PREFIX_PATH="${SRCDIR}/no-pdi" "${SRCDIR}/example"
make ${MAKEFLAGS}
ctest --output-on-failure --timeout 90 --output-junit /tmp/tests_no-pdi_example.xml -R PDI_example_trace_C
Copy link
Member

@jbigot jbigot Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why only run PDI_example_trace_C?

bin/build_and_run_all_tests
ctest --output-on-failure --timeout 90 ${CTEST_FLAGS} ${EXCLUDED_PDI_TESTS:+-E $EXCLUDED_PDI_TESTS}
ctest --output-on-failure --timeout 90 --output-junit /tmp/tests_pdi.xml ${EXCLUDED_PDI_TESTS:+-E $EXCLUDED_PDI_TESTS}

# Configure, build & test for pdi's example with CMAKE_PREFIX_PATH (find_package) for no-pdi, only with Paraconf
Copy link
Member

@jbigot jbigot Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Configure, build & test for pdi's example with CMAKE_PREFIX_PATH (find_package) for no-pdi, only with Paraconf
# Configure, build & test for pdi's example with CMAKE_PREFIX_PATH (find_package) for no-pdi, only if Paraconf is available

bin/build_and_run_all_tests
cd "${TEST_DIR_API}"
cmake -DCMAKE_PREFIX_PATH="${SRCDIR}/no-pdi" "${SRCDIR}/test_api"
make ${MAKEFLAGS}
ctest --output-on-failure --timeout 90 --output-junit /tmp/tests_no-pdi_api.xml
Copy link
Member

@jbigot jbigot Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ctest --output-on-failure --timeout 90 --output-junit /tmp/tests_no-pdi_api.xml
ctest --output-on-failure --timeout 90 ${CTEST_FLAGS} ${JUNIT_OUTPUT_DIR:+--output-junit ${JUNIT_OUTPUT_DIR}/tests_no-pdi_api.xml} ${EXCLUDED_PDI_TESTS:+-E $EXCLUDED_PDI_TESTS}

test_api/CHANGELOG.md
Copy link
Member

@jbigot jbigot Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's a changelog, there should also be a AUTHORS file

test_api/CHANGELOG.md
Comment on lines +21 to +22

## [x.x.x] - xxxx-xx-xx
Copy link
Member

@jbigot jbigot Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
## [x.x.x] - xxxx-xx-xx

test_api/CMakeLists.txt
find_package(PDI REQUIRED COMPONENTS C)

add_executable(test_api_C test_api.c)
if(paraconf_FOUND)
Copy link
Member

@jbigot jbigot Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if(paraconf_FOUND)
if("${paraconf_FOUND}")

test_api/CMakeLists.txt
add_test(NAME test_api_C COMMAND "$<TARGET_FILE:test_api_C>")

add_executable(test_api_CXX test_api.cpp)
if(paraconf_FOUND)
Copy link
Member

@jbigot jbigot Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if(paraconf_FOUND)
if("${paraconf_FOUND}")

tests/CMakeLists.txt
include(CTest)

# PDI
find_package(paraconf COMPONENTS C)
Copy link
Member

@jbigot jbigot Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it used Directly?

If yes, it should be REQUIRED and the target should be added as dependency

If no, this line can be removed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jbigot jbigot jbigot left review comments

@jmorice91 jmorice91 Awaiting requested review from jmorice91

@benoitmartin88 benoitmartin88 Awaiting requested review from benoitmartin88 benoitmartin88 is a code owner

@Yushan-Wang Yushan-Wang Awaiting requested review from Yushan-Wang Yushan-Wang is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees

@benoitmartin88 benoitmartin88

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Provide a mechanism to deactivate PDI on demand

6 participants

@JAuriac @jbigot @benoitmartin88 @Yushan-Wang @jmorice91